-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: fix content-type detection by doing a fall-back based on the ext… #1482
Conversation
//cc @vasco-santos @diasdavid This is related with ipfs-inactive/old-js-ipfs-website#122 because the SVGs aren't being rendered correctly when using the JS IPFS Gateway or when enabling the service-worker due to missing content-type header. |
093672e
to
95c6cb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - waiting on CI
@@ -12,6 +12,21 @@ const Stream = require('readable-stream') | |||
const { resolver } = require('ipfs-http-response') | |||
const PathUtils = require('../utils/path') | |||
|
|||
function detectContenType (ref, chunk) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo here detectContenType
- missing "t" on "conten"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix once the workshop I’m attending on dweb ends. :p
|
||
// try to guess the filetype based on the first bytes | ||
// note that `file-type` doesn't support svgs, thereore we assume it's a svg if ref looks like it | ||
if (!ref.endsWith('.svg')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, file-type doesn’t support svgs and might actually return invalid mine types. They state that in their readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw there’s already a comment there sorry!
95c6cb0
to
d69d52a
Compare
The typo should be fixed! |
The JS IPFS gateway was only responding with the correct content-type for some mime-types, see https://github.com/sindresorhus/file-type#supported-file-types. This commit now fall-backs to detecting based on the extension as well. Note that SVGs aren't supported by the `file-type` module.
d69d52a
to
49fedb6
Compare
Of note: ipfs/kubo#4025 |
The JS IPFS gateway was only responding with the correct content-type for some mime-types, see https://github.com/sindresorhus/file-type#supported-file-types.
This commit now fall-backs to detecting based on the extension as well.
Note that SVGs aren't supported by the
file-type
module.